Add independent ciper and MAC algorithms negotiation for each direction by yosuke-wolfssl · Pull Request #952 · wolfSSL/wolfssh

yosuke-wolfssl · 2026-04-24T00:22:56Z

This PR adds independent ciper and MAC algorithms negotiation for each direction to comply with RFC 4253 section 7.1.

Add peerEncryptId, peerMacId, peerAeadMode, peerBlockSz, peerMacSz into HandshakeInfo.
Fix DoKexInit() so that this matches each algorithm independently and store the results into each member in HandshakeInfo.
Fix DoNewKeys() so that this updates peer algorithm info like ssh->peerEncryptId with ssh->handshake->peerEncryptId and so on.

Also, new regress test for the case is added as well.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds RFC 4253 §7.1-compliant, per-direction cipher/MAC negotiation by tracking and applying independent peer vs local (C2S vs S2C) algorithm selections during KEXINIT/NEWKEYS processing, with a new regression test covering mixed-direction negotiation scenarios.

Changes:

Extend HandshakeInfo to store peer (opposite direction) cipher/MAC/AEAD and sizes independently.
Update DoKexInit() to negotiate cipher/MAC independently per direction and populate the new handshake fields.
Update DoNewKeys() to copy negotiated peer-direction algorithm selections from the handshake into the session, and add a regression test validating negotiation outcomes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`wolfssh/internal.h`	Adds per-direction (peer) negotiated cipher/MAC/AEAD fields to `HandshakeInfo`.
`src/internal.c`	Implements independent per-direction algorithm matching and propagates peer settings on `NEWKEYS`.
`tests/regress.c`	Adds a regression test ensuring negotiation differs per direction and handles AEAD vs non-AEAD correctly.

Comments suppressed due to low confidence (1)

src/internal.c:4486

The KEXINIT parsing now duplicates the same 'convert configured algo name-list into ID list' pattern multiple times (cipher S2C, MAC C2S, MAC S2C). Consider extracting a small helper to build cannedList/cannedListSz from ssh->algoListCipher / ssh->algoListMac to reduce repetition and the chance of future divergence between the per-direction negotiation blocks.

    if (ret == WS_SUCCESS && !ssh->handshake->peerAeadMode) {
        cannedAlgoNamesSz = AlgoListSz(ssh->algoListMac);
        cannedListSz = (word32)sizeof(cannedList);
        ret = GetNameListRaw(cannedList, &cannedListSz,
                (const byte*)ssh->algoListMac, cannedAlgoNamesSz);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…on, And add regress test

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot · 2026-04-24T17:22:17Z

        }
    }
+    if (ret == WS_SUCCESS) {
+        ssh->handshake->peerEncryptId = algoId;


🔴 [High] DoKexInit stores peer/local algo info in wrong struct members for client endpoints · Logic errors

handshake->peerKeys/peerEncryptId/peerAeadMode/peerBlockSz/peerMacId/peerMacSz are now unconditionally filled from the C2S negotiation and handshake->keys etc. from S2C. However, GenerateKeys() (src/internal.c:2666-2673) and the ssh->peerKeys/ssh->keys…

Fix: Select which fields receive C2S vs S2C results based on ssh->ctx->side: clients must put S2C into peerKeys/peerEncryptId/... and C2S into…

aidangarske

Skoll Code Review

Scan type: review-security
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[High] GenerateKeys skips both-direction MAC key derivation when only one direction is AEAD — src/internal.c:2696-2709
[Low] Test coverage asserts DoKexInit parse state only, not key derivation / data path for split-AEAD — tests/regress.c:2142-2210
[Info] HandshakeInfoNew skips peerMacSz/peerAeadMode initialization (safe due to WMEMSET but easy to miss) — src/internal.c:572-590

Review generated by Skoll

aidangarske · 2026-04-24T18:53:34Z

            ret = WS_MATCH_ENC_ALGO_E;
        }
    }
+    if (ret == WS_SUCCESS) {


🔴 [High] GenerateKeys skips both-direction MAC key derivation when only one direction is AEAD · Logic

GenerateKeys derives MAC key 'E' (C2S) and 'F' (S2C) inside a single if (!ssh->handshake->aeadMode) gate. Before this PR, DoKexInit enforced that the S2C algorithms had to match the already-selected C2S algorithms (the old code passed &algoId, 1 as the canned list when matching S2C enc and S2C MAC), so a single aeadMode flag correctly described both directions. After this PR, each direction is negotiated independently, and handshake->aeadMode now only reflects the S2C direction (peerAeadMode reflects C2S). When C2S is non-AEAD and S2C is AEAD — e.g. exactly the configuration exercised by the new TestIndependentAlgoNegotiation sub-test B (aes128-cbc C2S + aes256-gcm@openssh.com S2C) — handshake->aeadMode == 1 causes the guard to be false and NEITHER 'E' nor 'F' is generated. The C2S/peer HMAC key (peerKeys.macKey on the server) therefore stays all-zero from HandshakeInfoNew's WMEMSET, and the first MAC verification of an inbound C2S packet will use a zero key and fail, tearing down the session. The new regress test does not catch this because it only asserts handshake field values after DoKexInit and never drives GenerateKeys or the data path. The condition must gate each direction independently (e.g. generate 'E' when the C2S direction is non-AEAD, 'F' when the S2C direction is non-AEAD).

Fix: Split the gate so 'E' (C2S MAC key) is generated when the C2S direction is non-AEAD and 'F' (S2C MAC key) is generated when the S2C direction is non-AEAD. On the server: gate 'E' on !peerAeadMode (C2S) and 'F' on !aeadMode (S2C); on the client these roles are swapped. Then extend the regress test beyond DoKexInit to verify GenerateKeys derives the right keys (or a higher-level test that completes keying and exchanges data) to keep this regression from reappearing.

aidangarske · 2026-04-24T18:53:34Z


+#if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \
+    && !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256)
+static void TestIndependentAlgoNegotiation(void)


🔵 [Low] Test coverage asserts DoKexInit parse state only, not key derivation / data path for split-AEAD · Missing Tests

TestIndependentAlgoNegotiation only drives wolfSSH_TestDoKexInit and asserts fields on ssh->handshake (e.g. peerEncryptId, encryptId, aeadMode). It does not drive GenerateKeys, DoNewKeys, or end-to-end encryption/MAC with the negotiated mixed ciphers. As a result, the GenerateKeys bug above (not generating a MAC key for the non-AEAD direction when the other direction is AEAD) is not caught even though sub-test B constructs exactly the offending configuration. This elevates the risk of future regressions in the split-AEAD area.

Fix: Extend the test (or add a companion test) that completes keying for a split-AEAD configuration and verifies that the correct MAC keys are derived (non-zero macKey with the expected macKeySz for the non-AEAD side, zero-sized MAC key on the AEAD side). A round-trip send/receive with HMAC verification would be ideal.

aidangarske · 2026-04-24T18:53:34Z

@@ -578,6 +578,9 @@ static HandshakeInfo* HandshakeInfoNew(void* heap)
        newHs->encryptId = ID_NONE;


⚪ [Info] HandshakeInfoNew skips peerMacSz/peerAeadMode initialization (safe due to WMEMSET but easy to miss) · Logic

The new initializer block sets peerEncryptId, peerMacId, and peerBlockSz, mirroring the existing local-side initializers. It does not set peerAeadMode or peerMacSz. Functionally this is fine because WMEMSET(newHs, 0, sizeof(HandshakeInfo)) zeros the struct first and the desired default for both fields is zero (non-AEAD, zero MAC size). This is purely a consistency/readability observation — future readers adding new peer fields may miss that WMEMSET already handles zero defaults, and the selective initialization becomes less symmetric with the local side. No action required for security.

Fix: Optional: for symmetry, omit the explicit = ID_NONE / = 0 lines that merely restate the memset defaults, or add the missing peerMacSz/peerAeadMode / macSz / aeadMode lines to document intent.

Note: Referenced line (src/internal.c:572-590) is outside the diff hunk. Comment anchored to nearest changed region.

yosuke-wolfssl self-assigned this Apr 24, 2026

Copilot AI review requested due to automatic review settings April 24, 2026 00:22

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread src/internal.c

Copilot started reviewing on behalf of yosuke-wolfssl April 24, 2026 00:35 View session

yosuke-wolfssl force-pushed the f_607 branch from 0c34602 to 5cc90fa Compare April 24, 2026 00:43

Add independent ciper and MAC algorithms negotiation for each directi…

47c9e57

…on, And add regress test

yosuke-wolfssl force-pushed the f_607 branch from 5cc90fa to 47c9e57 Compare April 24, 2026 00:59

yosuke-wolfssl assigned wolfSSL-Bot and unassigned yosuke-wolfssl Apr 24, 2026

padelsbach requested a review from wolfSSL-Fenrir-bot April 24, 2026 17:11

wolfSSL-Fenrir-bot reviewed Apr 24, 2026

View reviewed changes

aidangarske reviewed Apr 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add independent ciper and MAC algorithms negotiation for each direction#952

Add independent ciper and MAC algorithms negotiation for each direction#952
yosuke-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
yosuke-wolfssl:f_607

yosuke-wolfssl commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

wolfSSL-Fenrir-bot Apr 24, 2026

Uh oh!

aidangarske left a comment

Uh oh!

aidangarske Apr 24, 2026

Uh oh!

aidangarske Apr 24, 2026

Uh oh!

aidangarske Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -578,6 +578,9 @@ static HandshakeInfo* HandshakeInfoNew(void* heap)
		newHs->encryptId = ID_NONE;

Conversation

yosuke-wolfssl commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

wolfSSL-Fenrir-bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

aidangarske Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

aidangarske Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants